-
-
Notifications
You must be signed in to change notification settings - Fork 181
FE: Optional FTS #1402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
FE: Optional FTS #1402
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces optional Full Text Search (FTS) functionality across the frontend application, allowing users to toggle between regular and full text search modes for various resources.
- Adds FTS UI components and hooks for managing FTS state
- Integrates FTS toggle functionality into search interfaces for topics, schemas, consumer groups, connectors, and ACLs
- Updates API hooks to support FTS parameter passing
- Enhances search component to support additional actions alongside the clear button
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
frontend/src/theme/theme.ts | Adds theme colors for FTS icon states |
frontend/src/lib/hooks/api/*.ts | Updates API hooks to accept and pass FTS parameter |
frontend/src/components/contexts/ClusterContext.ts | Adds FTS configuration properties to cluster context |
frontend/src/components/common/Tooltip/Tooltip.tsx | Adds fullWidth prop for tooltip styling |
frontend/src/components/common/Search/* | Refactors search component to support additional actions |
frontend/src/components/common/Icons/FtsIcon.tsx | Adds FTS toggle icon component |
frontend/src/components/common/Fts/* | Implements FTS hook and component for state management |
frontend/src/components//List/ | Integrates FTS functionality into list pages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
width: 'max-content', | ||
minWidth: fullWidth ? 'max-content' : 'initial', |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minWidth
property logic seems inverted. When fullWidth
is true, setting minWidth: 'max-content'
doesn't achieve full width behavior. Consider using width: '100%'
or removing the minWidth property when fullWidth is true.
width: 'max-content', | |
minWidth: fullWidth ? 'max-content' : 'initial', | |
width: fullWidth ? '100%' : 'max-content', | |
minWidth: 'initial', |
Copilot uses AI. Check for mistakes.
search?: string, | ||
fts?: boolean | ||
) => { | ||
const base = ['clusters', clusterName, 'connectors', fts, 'fts']; |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query key includes both the fts
boolean value and the string 'fts', which creates inconsistent cache keys. When fts
is undefined, the key becomes [..., undefined, 'fts']
. Consider using a more structured approach like { fts }
or only include 'fts' when the feature is enabled.
Copilot uses AI. Check for mistakes.
|
||
return { | ||
handleSwitch, | ||
isFtsFetureEnabled: isFtsFeatureEnabled, |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'isFtsFetureEnabled' to 'isFtsFeatureEnabled'.
isFtsFetureEnabled: isFtsFeatureEnabled, | |
isFtsFeatureEnabled: isFtsFeatureEnabled, |
Copilot uses AI. Check for mistakes.
{...inputOptions} | ||
/> | ||
{clearIcon} | ||
{search && clearIcon} |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional rendering of clearIcon
only when search
is true may break existing functionality. The clearIcon
prop was previously always rendered regardless of the search
prop. This could be a breaking change if components rely on clearIcon
being displayed in non-search contexts.
{search && clearIcon} | |
{clearIcon} |
Copilot uses AI. Check for mistakes.
ftsEnabled: false, | ||
ftsDefaultEnabled: false, |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FTS properties are hardcoded to false
in ClusterPage. This suggests the FTS feature configuration is not yet properly integrated from the backend cluster data. Consider implementing proper feature detection based on cluster capabilities.
ftsEnabled: false, | |
ftsDefaultEnabled: false, | |
ftsEnabled: features.includes(ClusterFeaturesEnum.FTS_ENABLED), | |
ftsDefaultEnabled: features.includes(ClusterFeaturesEnum.FTS_DEFAULT_ENABLED), |
Copilot uses AI. Check for mistakes.
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
Check out Contributing and Code of Conduct
A picture of a cute animal (not mandatory but encouraged)